Enable strict compiler warnings for C and Cython levels#9718
Enable strict compiler warnings for C and Cython levels#9718Tawfeeqshaik wants to merge 11 commits into
Conversation
|
Hi @ThomasWaldmann! I've opened this PR to resolve #9140 by adding stricter build-level warnings. I ended up implementing changes across both the C flag array and the Cython compiler options. Please let me know if you'd like me to add or modify any specific flags, or if there are any further adjustments needed to get this ready for a merge. Looking forward to your feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9718 +/- ##
==========================================
+ Coverage 83.55% 83.61% +0.05%
==========================================
Files 93 93
Lines 15681 15700 +19
Branches 2353 2360 +7
==========================================
+ Hits 13102 13127 +25
+ Misses 1838 1831 -7
- Partials 741 742 +1 ☔ View full report in Codecov by Harness. |
|
@Tawfeeqshaik Thanks for the PR. Wondering about platform compatibility of such options: are they the same on every platform, linux, bsd, macOS, omniOS, haiku - but not on windows? |
@ThomasWaldmann, thanks for taking a look at this! Yep, they're fully compatible. Since Linux, macOS, the BSDs, omniOS, and Haiku all rely on GCC or Clang as their default compilers, they'll handle As for the Cython directives ( |
|
https://github.com/borgbackup/borg/actions/runs/26938203010/job/79473456121?pr=9718 Quite a lot of warnings now. Guess we need to evaluate what's actually useful for borg and what is just spam (because it is either harmless or up to another project to fix). |
Thanks for checking it further! Yep, that makes total sense , enabling strict warnings can definitely surface a lot of existing noise across dependencies and older code paths. It’s cool to see the compiler digging deep, but we definitely want to avoid cluttering the build logs with unactionable noise. Just let me know which specific flags you'd like me to drop or adjust, and I'll update the PR right away! |
|
There are tons of "implicit declaration of '...'" warnings. E.g.: warning: src/borg/chunkers/buzhash.pyx:6:7: implicit declaration of 'time' Line 6 is just: So, what does it even want from us? :-) |
Ah, got it! That's Cython's code generation triggering a C-level warning because it's missing the explicit header includes (like I can fix those cleanly by adding |
|
Hi @ThomasWaldmann, just wanted to let you know that the latest changes have successfully passed all of the CI linting and platform test suites! The build logs look crisp and focused now. Whenever you have a moment, it should be fully ready for your final review and merge. Thanks again for the helpful guidance through this! |
|
Well, the "implicit declaration" stuff still looks a bit spammy, did you see that? |
|
Did you just |
Ah, you're right I accidentally committed my local venv files while fixing the formatting issue. I've cleaned up the PR and pushed only the relevant changes now. |
| @@ -48,6 +48,9 @@ | |||
| # Extra cflags for all extensions, usually just warnings we want to enable explicitly | |||
| cflags = ["-Wall", "-Wextra", "-Wpointer-arith", "-Wno-unreachable-code-fallthrough"] | |||
There was a problem hiding this comment.
From the logs:
cc1: note: unrecognized command-line option ‘-Wno-unreachable-code-fallthrough’ may have been intended to silence earlier diagnostics
That seems to be a pre-existing issue, but maybe you could please check that also? Was that option removed? Is the option name different?
There was a problem hiding this comment.
Ya . -Wno-unreachable-code-fallthrough isn't actually a valid flag combination for GCC or Clang, which is why the compiler was scratching its head and throwing that diagnostic note.
I just went ahead and swapped it out for the correct flag, -Wno-implicit-fallthrough. That should completely clean up that pre-existing noise from the logs on this run!
Description
Fixes #9140
This PR addresses the issue by implementing stricter compiler warnings and checks across both the C and Cython build layers:
-Wpedanticand-Wstrict-prototypesto the centralcflagsarray (safely wrapping it to skip Windows/MSVC compiler configurations).warn.undeclaredandwarn.unreachableto thecompiler_directivesdictionary inside thecythonizeblock to catch un-declared variables and dead code early.Checklist
master(or maintenance branch if only applicable there)toxor the relevant test subset)